Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace node::MakeCallback with AsyncResource call #26

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

taoyuan
Copy link

@taoyuan taoyuan commented Jun 12, 2018

  • Fix GetRealNamedProperty build error in cpp
  • Replace node::MakeCallback with AsyncResource call

@taoyuan
Copy link
Author

taoyuan commented Jun 14, 2018

This PR fixed #24

@IjzerenHein
Copy link

I can confirm that this works. Using this pull request on Node 10 resolves the issue.

@sandeepmistry Can we get this merged?

@OtaK
Copy link

OtaK commented Jun 28, 2018

Same here, I confirm that it fixes the problems on macOS. Would be amazing it if gets merged ASAP.

@cehoffman
Copy link

Confirm this fixed compile for me on 10.3.4 OS X and 10.7.0 NodeJS

@LeviSchuck
Copy link

Any news on this?

@coenmooij
Copy link

@sandeepmistry could you please take a look?

@jdpigeon
Copy link

Please merge! 👍

@ticky
Copy link

ticky commented Sep 28, 2018

@sandeepmistry sorry to ping you directly but a merge on this would be much appreciated!

@chrisdothtml
Copy link

Not sure why exactly the owner hasn't merged this, but it may have something to do with all the unrelated changes in this PR. Things like:

  • adding .idea to the .gitignore (when that should probably just be in your global ignore)
  • adding additional binding dependency
  • renaming the target file

I could be wrong, and the changes may actually be necessary for the fix, but in my experience, when a PR has extra unrelated "cleanup" changes, it can discourage the maintainer from wanting to merge it

@jongear
Copy link

jongear commented Jan 16, 2019

Since this repo appears to be abandoned, I took the liberty of cloning over the existing repo and implementing this PR for a package I called xpc-connect. The package is available on npm to use

npm install xpc-connect
https://www.npmjs.com/package/xpc-connect

Thanks so much for this @taoyuan 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.